-
Notifications
You must be signed in to change notification settings - Fork 3
feat: req/resp logging middleware #105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
beb1aca to
d021c47
Compare
|
|
||
| resp, err := next(req) | ||
| if err != nil { | ||
| return resp, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about it but maybe it be useful to have "err" file with error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave this for a future enhancement, ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, could you please create an issue about it?
bridge_integration_test.go
Outdated
| reqDumpData, err := os.ReadFile(reqDumpFile) | ||
| require.NoError(t, err, "request dump file should exist") | ||
| require.Contains(t, string(reqDumpData), "POST ") | ||
| require.Contains(t, string(reqDumpData), "Host:") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add check like require.Contains(t, string(reqDumpData), string(reqBody))
and similar for response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This forced me to confront the fact that map keys were not stably sorted. Thanks 👍
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ec13727 to
db0f11e
Compare
|
|
||
| for _, key := range headerKeys { | ||
| // Skip Content-Length since we write it explicitly above with the pretty-printed body length. | ||
| if key == "Content-Length" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Host could also be skipped
| // Sort header keys for deterministic output. | ||
| headerKeys := make([]string, 0, len(resp.Header)) | ||
| for key := range resp.Header { | ||
| headerKeys = append(headerKeys, key) | ||
| } | ||
| slices.Sort(headerKeys) | ||
|
|
||
| for _, key := range headerKeys { | ||
| _, sensitive := sensitiveResponseHeaders[key] | ||
| for _, value := range resp.Header[key] { | ||
| if sensitive { | ||
| value = redactHeaderValue(value) | ||
| } | ||
| fmt.Fprintf(&headerBuf, "%s: %s\r\n", key, value) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks copy-pasty from lines 105-124.
| return []aibridge.Provider{provider.NewOpenAI(openaiCfgWithAPIDump(addr, apiKey, dumpDir))} | ||
| }, | ||
| createRequestFunc: createOpenAIChatCompletionsReq, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test case for responses
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
Signed-off-by: Danny Kopping <[email protected]>
a93b2ec to
e4c2a82
Compare

Logs the raw HTTP requests to and responses from AI providers' APIs.